support dependencies as git url with exact commit #1765

belka-ew · 2019-09-02T14:42:16Z

Revival of [WIP] Support dependencies as git url (including with exact commit hash).
The PR introduces new version object type. Dependencies example:

{
	"name": "dub-git",

	"dependencies": {
		"tanya": {
			"repository": "https://github.com/caraus-ecms/tanya",
			"version": "73535568b79a0b124bc1653002637a830ce0fcb8"
		}
	}
}

Also ~branch as version works.

dlang-bot · 2019-09-02T14:42:17Z

Thanks for your pull request and interest in making D better, @belka-ew! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

andre2007 · 2019-09-02T18:15:37Z

@belka-ew great that you are working in this topic. I am not really sure how the exact syntax could look like. Did you considered:

"dependencies": {
		"tanya": {
			"url": "git+https://github.com/caraus-ecms/tanya#73535568b79a0b124bc1653002637a830ce0fcb8"
		}
	}

(I am not really an expert on this topic, don't know wheter this git syntax makes sense, but it is somehow consistent to the maven syntax used in Dub)

lesderid · 2019-09-02T18:16:55Z

Does this work with local git repository paths too?

andre2007 · 2019-09-02T20:11:43Z

@lesderid I am not sure wheter the question was meant for @belka-ew or for me. Yes, the protocol would be git+file (here some details from npm https://docs.npmjs.com/files/package.json#git-urls-as-dependencies)

lesderid · 2019-09-02T20:14:03Z

@lesderid I am not sure wheter the question was meant for @belka-ew or for me. Yes, the protocol would be git+file (here some details from npm https://docs.npmjs.com/files/package.json#git-urls-as-dependencies)

Sorry, was meant for @belka-ew

belka-ew · 2019-09-03T08:02:09Z

@belka-ew great that you are working in this topic. I am not really sure how the exact syntax could look like. Did you considered:
"dependencies": {
		"tanya": {
			"url": "git+https://github.com/caraus-ecms/tanya#73535568b79a0b124bc1653002637a830ce0fcb8"
		}
	}
(I am not really an expert on this topic, don't know wheter this git syntax makes sense, but it is somehow consistent to the maven syntax used in Dub)

This is doable, but I don't like the syntax. Dub already supports objects in version specification for paths "mypackage": { "path": "..." }, so why not reuse it. It is more readable and easier to remember than magic characters like #; different things have different entries (repository for the url and version for the git reference).

npm keeps everything in one string and introduces a magic notation for different elements #v1.0.27, #semver:^5.0 and so on.

belka-ew · 2019-09-03T08:03:43Z

Does this work with local git repository paths too?

Do you mean a bare git repository locally? The code just does git clone, so yes, whatever git clone accepts, should work here.

lesderid · 2019-09-04T09:51:12Z

Do you mean a bare git repository locally? The code just does git clone, so yes, whatever git clone accepts, should work here.

The code is using URL for the repository, so you'd have to use file://, which only works with absolute paths.

joseph-wakeling-frequenz · 2019-09-16T17:34:54Z

@belka-ew thanks for reviving this idea. A few thoughts and queries:

(in response to others' feedback) let's not mess around with inventing magic syntaxes: git's own URL syntax for remotes should be matched exactly (with all possible options supported)
- we should clearly differentiate between the repo and the version of the code to check out
(immediately messing around with syntax) if we go with "repository" : "..." are we going to run into issues if we want to add support for more repository types in the future (e.g. mercurial, bzr/brz, fossil, SVN, ...)? Would it be better to have "git" : "{URL}" syntax?
- note, there's not really a blocker here, as in future we could always add a "vcs" key to specify the type of repo ("vcs" : "git", "vcs" : "bzr", etc.) but it would be nice to think a little about this ahead of time
care should be taken to test this with repo providers other than GitHub
- ideally I'd suggest integration tests with at least: GitHub, GitLab, BitBucket, Launchpad (I'm sure others can suggest extra options)
relative/local URLs like ../my-other-project/ should be supported
- this may be important for access to resources in some CI systems, see e.g. GitLab's requirements for referring to URLs of private projects: https://docs.gitlab.com/ee/ci/git_submodules.html (they're talking about submodules but the same URL constraints may apply)
IIUC version can currently be specified by either commit hash or branch, what about tag?

belka-ew · 2019-09-21T15:22:54Z

I made the PR work with local paths as well. Now it is really the case, that everything git clone accepts should work, because I save the path in a string and leave git to decide what is a correct path/address and what is not.

@joseph-wakeling-frequenz, I’m not sure about the tag support. The tags just reference specific commits, so they can be replaced with the hash in 100% of cases. The problem aren’t the tags themselves, but I want really to reuse “version”: “...” to have more compact dependency definition and because it already has support for branches (probably the most common case for this kind of dependency specification). The version in dub is currently checked against some conditions: whether it is SemVer, a branch or hash. If I would support any git references (like tags), I have to accept any string as version and have to remove all these checks - and it would affect the current logic for normal dependencies.

belka-ew · 2019-09-25T04:05:57Z

Can I get some official feedback here? What has to be done to get it merged?

belka-ew · 2019-10-08T07:13:37Z

ping

belka-ew · 2019-10-08T07:44:06Z

@thewilsonator, @andre2007, @lesderid, @joseph-wakeling-frequenz thanks.

wilzbach · 2019-10-08T08:54:27Z

I don't have time to dive into this, but

no changelog
no tests
no human reviews
real code problems like that this should have been a PackageSupplier instead of patching itself through the entire codebase

@thewilsonator !!!. I'm now going to revert as we have a clear policy that we don't merge things that don't have e.g. tests.

belka-ew · 2019-10-08T09:13:15Z

It is not a PackageSuppliert, git repositories are single packages like paths. Things like PackageSupplier.getVersions() don't make sense for git.

joseph-wakeling-frequenz

It may be a bit late now, but since there's some controversy over the commit, I thought I'd add some detailed feedback.

As an extra stylistic note, there appears to be a bit of a mix-up of tabs and spaces for indentation in this commit. That may be following existing inconsistencies between files, though.

joseph-wakeling-frequenz · 2019-10-08T09:10:59Z

source/dub/dependency.d

@@ -93,11 +93,28 @@ struct Dependency {
 		m_path = path;
 	}

+	this(Repository repository, string spec) {


Missing docs for this constructor overload.

joseph-wakeling-frequenz · 2019-10-08T09:11:15Z

source/dub/dependency.d

@@ -111,6 +128,8 @@ struct Dependency {
 	/// Returns true $(I iff) the version range only matches a specific version.
 	@property bool isExactVersion() const { return m_versA == m_versB; }

+	@property bool isGit() const { return !repository.empty; }


Missing docs on this new property.

joseph-wakeling-frequenz · 2019-10-08T09:13:57Z

source/dub/dependency.d

+		if (!repository.empty) {
+			ret ~= "#";
+		}
+		ret ~= versionSpec;


Not an expert on all the options here, but this formulation looks questionable: is it legit for the git version spec to be followed by extra stuff (which will happen if optional is true, or if !path.empty)?

Also, why the # symbol?

joseph-wakeling-frequenz · 2019-10-08T09:14:40Z

source/dub/dependency.d

@@ -397,6 +425,7 @@ struct Dependency {
 		A specification is valid if it can match at least one version.
 	*/
 	bool valid() const {
+		if (isGit) return true;


I suggest being consistent in usage: this.isGit (as elsewhere), not just isGit

joseph-wakeling-frequenz · 2019-10-08T09:16:02Z

source/dub/dependency.d

@@ -706,7 +774,7 @@ struct Version {
 		Note that branches are always considered pre-release versions.
 	*/
 	@property bool isPreRelease() const {
-		if (isBranch) return true;
+		if (isBranch || isGit) return true;


Why should isGit necessarily imply a pre-release? What if the git version spec points to a commit with a release tag?

joseph-wakeling-frequenz · 2019-10-08T09:16:39Z

source/dub/dependency.d

+	assert(Version("73535568b79a0b124bc1653002637a830ce0fcb8").isGit);
+}
+
+private bool isHash(string hash) @nogc nothrow pure @safe


Missing function documentation

joseph-wakeling-frequenz · 2019-10-08T09:17:02Z

source/dub/project.d

@@ -1542,6 +1550,17 @@ final class SelectedVersions {
 		m_dirty = true;
 	}

+	void selectVersion(string package_id, Repository repository, string spec)


missing method documentation

joseph-wakeling-frequenz · 2019-10-08T09:17:11Z

source/dub/project.d

@@ -1604,16 +1623,23 @@ final class SelectedVersions {

 	static Json dependencyToJson(Dependency d)


missing method docs

wilzbach · 2019-10-22T06:43:28Z

@belka-ew I hope I didn't discourage you too much from this. My motivation to revert was solely based on the fact that I think it was merged prematurely (no reviews, no tests). It was not a vote against this feature.

Things like PackageSupplier.getVersions() don't make sense for git.

Why not? You can list all tags of a git repository, e.g.

git ls-remote --tags https://github.com/dlang/dub | sed -n 's|.*refs/tags/\(v\?[0-9]*\.[0-9]*\.[0-9]*$\)|\1|p' | sort --version-sort

belka-ew · 2019-10-22T07:16:29Z

Why not? You can list all tags of a git repository, e.g.

Tags aren't very useful since in the most cases they are versions that can be obtained from the registry. The most useful use case is probably branches, if I want to test a library before tagging it and without waiting for a registry update. But also specific commits are possible, and the amount of commits can be huge.

joseph-wakeling-frequenz · 2019-10-22T09:38:36Z

Tags aren't very useful since in the most cases they are versions that can be obtained from the registry.

I think you're missing a convenience use-case here. If I'm specifying the version of a dependency to use, it's much nicer if I can use human semantic versioning, rather than Git's hash-based commit IDs.

DUB config files need to be human-readable first.

belka-ew force-pushed the pr_vesion_commit2 branch from 995ddac to fd1ef8d Compare September 3, 2019 07:40

belka-ew force-pushed the pr_vesion_commit2 branch from fd1ef8d to c1ae7b9 Compare September 21, 2019 14:56

Support dependencies as git url with exact commit

38fbe4c

belka-ew force-pushed the pr_vesion_commit2 branch from c1ae7b9 to 38fbe4c Compare September 21, 2019 16:09

thewilsonator approved these changes Oct 8, 2019

View reviewed changes

thewilsonator added the auto-merge label Oct 8, 2019

dlang-bot merged commit 24a2ef9 into dlang:master Oct 8, 2019

belka-ew deleted the pr_vesion_commit2 branch October 8, 2019 07:37

wilzbach mentioned this pull request Oct 8, 2019

Revert "support dependencies as git url with exact commit" #1782

Merged

joseph-wakeling-frequenz reviewed Oct 8, 2019

View reviewed changes

belka-ew mentioned this pull request Nov 20, 2019

Support dependencies as git url with exact commit #1798

Merged

Geod24 mentioned this pull request Dec 27, 2019

Support git submodules as dependencies #1840

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support dependencies as git url with exact commit #1765

support dependencies as git url with exact commit #1765

belka-ew commented Sep 2, 2019 •

edited

Loading

dlang-bot commented Sep 2, 2019

andre2007 commented Sep 2, 2019

lesderid commented Sep 2, 2019

andre2007 commented Sep 2, 2019

lesderid commented Sep 2, 2019

belka-ew commented Sep 3, 2019

belka-ew commented Sep 3, 2019

lesderid commented Sep 4, 2019

joseph-wakeling-frequenz commented Sep 16, 2019 •

edited

Loading

belka-ew commented Sep 21, 2019

belka-ew commented Sep 25, 2019

belka-ew commented Oct 8, 2019

belka-ew commented Oct 8, 2019

wilzbach commented Oct 8, 2019 •

edited

Loading

belka-ew commented Oct 8, 2019

joseph-wakeling-frequenz left a comment

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

joseph-wakeling-frequenz Oct 8, 2019

wilzbach commented Oct 22, 2019

belka-ew commented Oct 22, 2019

joseph-wakeling-frequenz commented Oct 22, 2019

		@@ -1604,16 +1623,23 @@ final class SelectedVersions {

		static Json dependencyToJson(Dependency d)

support dependencies as git url with exact commit #1765

support dependencies as git url with exact commit #1765

Conversation

belka-ew commented Sep 2, 2019 • edited Loading

dlang-bot commented Sep 2, 2019

andre2007 commented Sep 2, 2019

lesderid commented Sep 2, 2019

andre2007 commented Sep 2, 2019

lesderid commented Sep 2, 2019

belka-ew commented Sep 3, 2019

belka-ew commented Sep 3, 2019

lesderid commented Sep 4, 2019

joseph-wakeling-frequenz commented Sep 16, 2019 • edited Loading

belka-ew commented Sep 21, 2019

belka-ew commented Sep 25, 2019

belka-ew commented Oct 8, 2019

belka-ew commented Oct 8, 2019

wilzbach commented Oct 8, 2019 • edited Loading

belka-ew commented Oct 8, 2019

joseph-wakeling-frequenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Oct 22, 2019

belka-ew commented Oct 22, 2019

joseph-wakeling-frequenz commented Oct 22, 2019

belka-ew commented Sep 2, 2019 •

edited

Loading

joseph-wakeling-frequenz commented Sep 16, 2019 •

edited

Loading

wilzbach commented Oct 8, 2019 •

edited

Loading